Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify and improve const-prop lints #69185

Merged
merged 16 commits into from
Feb 20, 2020
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 15, 2020

Add a single helper method for all lints emitted by const-prop, and make that lint different from the CTFE const_err lint. Also consistently check overflow on arithmetic, not on the assertion, to make behavior the same for debug and release builds.

See this summary comment for details and the latest status.

In terms of lint formatting, I went for what seems to be the better style: have a general message above the code, and then a specific message at the span:

error: this arithmetic operation will overflow
  --> $DIR/const-err2.rs:21:18
   |
LL |     let a_i128 = -std::i128::MIN;
   |                  ^^^^^^^^^^^^^^^ attempt to negate with overflow

We could also just have the specific message above and no text at the span if that is preferred.

I also converted some of the existing tests to use compiletest revisions, so that the same test can check a bunch of different compile flags.

Fixes #69020.
Helps with #69021: debug/release are now consistent, but the assoc-const test in that issue still fails (there is a FIXME in the PR for this). The reason seems to be that const-prop notices the assoc const in T::N << 42 and does not even bother calling const_prop on that operation.
Has no effect on #61821; the duplication there has entirely different reasons.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2020
@RalfJung
Copy link
Member Author

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Feb 15, 2020
@@ -2,33 +2,34 @@
// optimized compilation and unoptimized compilation and thus would
// lead to different lints being emitted

// revisions: default noopt opt opt_with_overflow_checks
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have gone overboard by having 4 revisions -- specifically, default (no extra flags) is likely a duplicate of one of the others, but I am not sure which. We already have so many tests, so I am somewhat inclined to remove default again... any opinions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like revisions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but there's such a thing as too much of any good thing. ;)


fn main() {
println!("{}", 0u32 - 1);
//[opt_with_overflow_checks,noopt]~^ WARN [overflow]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it look like default is the same as opt, which I find surprising.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

src/librustc_session/lint/builtin.rs Show resolved Hide resolved
src/librustc_session/lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_session/lint/builtin.rs Outdated Show resolved Hide resolved
@@ -2,33 +2,34 @@
// optimized compilation and unoptimized compilation and thus would
// lead to different lints being emitted

// revisions: default noopt opt opt_with_overflow_checks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like revisions

src/test/compile-fail/consts/const-err3.rs Show resolved Hide resolved
Co-Authored-By: Mazdak Farrokhzad <[email protected]>
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member Author

Man, is there any part of the test suite that does not check these lints?^^

@RalfJung
Copy link
Member Author

I now picked arithmetic_overflow and unconditional_panic as lint names. I also decided, because this PR already renames lints, it is better to do all renaming at once, so exceeding_bitshifts is now also covered by arithmetic_overflow. However, the lint level remains the same: deny-by-default.


So, in case you want to FCP or otherwise want a summary: what this PR does is to take some of the lints that were so far reported as const_err, and move them to a different category. const_err ought to be for hard errors during const-eval, but we were also using it for when we determined that some run-time code was likely buggy (e.g. because it has overflowing arithmetic or an operation that will always panic, such as div-by-0). This analysis is performed as a side-effect of const-prop (not cons-eval).
Those cases (and only those) are now reported as arithmetic_overflow and unconditional_panic, respectively. It also turns out that const-prop already used a dedicated lint for one particular kind of overflow, namely exceeding_bitshifts. So this PR also changes those lints to be emitted as arithmetic_overflow, because it seems odd to have one lint per operation and now that we are touching the lints anyway we might as well do something that makes sense.

In other words, no new lints get emitted by this PR, but the following changes:

  • Parts of const_err become unconditional_panic.
  • Other parts of const_err become arithmetic_overflow.
  • exceeding_bitshifts becomes arithmetic_overflow.

With this done, one rarely wants to allow const_err (this lint is now only fired for constants that cannot be used because their evaluation failed [including promoteds]); the only reason that is a lint and not a hard error is backwards-compatibility. The other lints however are more like our regular diagnostics. I think we should make them warn-by-default instead of deny-by-default, but for now this PR leaves the default levels unchanged (but I am happy to adjust in this PR or a follow-up).

@Centril
Copy link
Contributor

Centril commented Feb 18, 2020

I'm happy with the new lint names. I don't think FCP for renaming necessary. I would like to leave the lint severities as-is though. The enumerated lints seem like a case of correctness class lints that highlight actual bugs, so I think stopping compilation (as achieved with deny-by-default) is warranted.

@RalfJung
Copy link
Member Author

All right. @oli-obk the PR is awaiting your review, then. :)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 19, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2020

📌 Commit 58bb47e has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2020
@bors
Copy link
Contributor

bors commented Feb 19, 2020

⌛ Testing commit 58bb47e with merge 46f7297f050cd21d50120779f5b06a7d3be72347...

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 19, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2020
@RalfJung
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Feb 19, 2020

📌 Commit 88d14bf has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2020
bors added a commit that referenced this pull request Feb 20, 2020
Rollup of 5 pull requests

Successful merges:

 - #68877 (On mismatched argument count point at arguments)
 - #69185 (Unify and improve const-prop lints)
 - #69305 (Tweak binding lifetime suggestion text)
 - #69311 (Clean up E0321 and E0322)
 - #69317 (Fix broken link to the rustc guide)

Failed merges:

r? @ghost
@bors bors merged commit d237e0f into rust-lang:master Feb 20, 2020
@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 21, 2020
@RalfJung
Copy link
Member Author

I marked this for relnotes, given the user-visible changes here. I hope that is appropriate.

@RalfJung RalfJung deleted the const-prop-lints branch February 21, 2020 10:02
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 16, 2020
Pkgsrc changes:
 * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the
   bootstrap is not (yet?) available.

Upstream changes:

Version 1.43.0 (2020-04-23)
==========================

Language
--------
- [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having
  the type inferred correctly.][68129]
- [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201]

**Syntax only changes**
- [Allow `type Foo: Ord` syntactically.][69361]
- [Fuse associated and extern items up to defaultness.][69194]
- [Syntactically allow `self` in all `fn` contexts.][68764]
- [Merge `fn` syntax + cleanup item parsing.][68728]
- [`item` macro fragments can be interpolated into `trait`s, `impl`s,
  and `extern` blocks.][69366]
  For example, you may now write:
  ```rust
  macro_rules! mac_trait {
      ($i:item) => {
          trait T { $i }
      }
  }
  mac_trait! {
      fn foo() {}
  }
  ```
These are still rejected *semantically*, so you will likely receive an error but
these changes can be seen and parsed by macros and
conditional compilation.


Compiler
--------
- [You can now pass multiple lint flags to rustc to override the previous
  flags.][67885] For example; `rustc -D unused -A unused-variables` denies
  everything in the `unused` lint group except `unused-variables` which
  is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies
  everything in the `unused` lint group **including** `unused-variables` since
  the allow flag is specified before the deny flag (and therefore overridden).
- [rustc will now prefer your system MinGW libraries over its bundled libraries
  if they are available on `windows-gnu`.][67429]
- [rustc now buffers errors/warnings printed in JSON.][69227]

Libraries
---------
- [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement
  `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>`
  respectively.][69538] **Note** These conversions are only available when `N`
  is `0..=32`.
- [You can now use associated constants on floats and integers directly, rather
  than having to import the module.][68952] e.g. You can now write `u32::MAX` or
  `f32::NAN` with no imports.
- [`u8::is_ascii` is now `const`.][68984]
- [`String` now implements `AsMut<str>`.][68742]
- [Added the `primitive` module to `std` and `core`.][67637] This module
  reexports Rust's primitive types. This is mainly useful in macros
  where you want avoid these types being shadowed.
- [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642]
- [`string::FromUtf8Error` now implements `Clone + Eq`.][68738]

Stabilized APIs
---------------
- [`Once::is_completed`]
- [`f32::LOG10_2`]
- [`f32::LOG2_10`]
- [`f64::LOG10_2`]
- [`f64::LOG2_10`]
- [`iter::once_with`]

Cargo
-----
- [You can now set config `[profile]`s in your `.cargo/config`, or through
  your environment.][cargo/7823]
- [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's
  executable path when running integration tests or benchmarks.][cargo/7697]
  `<name>` is the name of your binary as-is e.g. If you wanted the executable
  path for a binary named `my-program`you would use
  `env!("CARGO_BIN_EXE_my-program")`.

Misc
----
- [Certain checks in the `const_err` lint were deemed unrelated to const
  evaluation][69185], and have been moved to the `unconditional_panic` and
  `arithmetic_overflow` lints.

Compatibility Notes
-------------------

- [Having trailing syntax in the `assert!` macro is now a hard error.][69548]
  This has been a warning since 1.36.0.
- [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly
  led to some instances being accepted, and now correctly emits a hard error.

[69340]: rust-lang/rust#69340

Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of `rustc` and
related tools.

- [All components are now built with `opt-level=3` instead of `2`.][67878]
- [Improved how rustc generates drop code.][67332]
- [Improved performance from `#[inline]`-ing certain hot functions.][69256]
- [traits: preallocate 2 Vecs of known initial size][69022]
- [Avoid exponential behaviour when relating types][68772]
- [Skip `Drop` terminators for enum variants without drop glue][68943]
- [Improve performance of coherence checks][68966]
- [Deduplicate types in the generator witness][68672]
- [Invert control in struct_lint_level.][68725]

[67332]: rust-lang/rust#67332
[67429]: rust-lang/rust#67429
[67637]: rust-lang/rust#67637
[67642]: rust-lang/rust#67642
[67878]: rust-lang/rust#67878
[67885]: rust-lang/rust#67885
[68129]: rust-lang/rust#68129
[68672]: rust-lang/rust#68672
[68725]: rust-lang/rust#68725
[68728]: rust-lang/rust#68728
[68738]: rust-lang/rust#68738
[68742]: rust-lang/rust#68742
[68764]: rust-lang/rust#68764
[68772]: rust-lang/rust#68772
[68943]: rust-lang/rust#68943
[68952]: rust-lang/rust#68952
[68966]: rust-lang/rust#68966
[68984]: rust-lang/rust#68984
[69022]: rust-lang/rust#69022
[69185]: rust-lang/rust#69185
[69194]: rust-lang/rust#69194
[69201]: rust-lang/rust#69201
[69227]: rust-lang/rust#69227
[69548]: rust-lang/rust#69548
[69256]: rust-lang/rust#69256
[69361]: rust-lang/rust#69361
[69366]: rust-lang/rust#69366
[69538]: rust-lang/rust#69538
[cargo/7823]: rust-lang/cargo#7823
[cargo/7697]: rust-lang/cargo#7697
[`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed
[`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html
[`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html
[`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html
[`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html
[`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate errors for overflowing division / remainder
6 participants